Skip to content

Conversation

AlexeySachkov
Copy link
Contributor

No description provided.

@AlexeySachkov AlexeySachkov requested a review from a team as a code owner June 13, 2025 13:15
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit.

@@ -378,19 +378,11 @@ group_ballot([[maybe_unused]] Group g, [[maybe_unused]] bool predicate) {
#ifdef __SYCL_DEVICE_ONLY__
return sycl::detail::commonGroupBallotImpl(g, predicate);
#else
throw exception{errc::feature_not_supported,
"Sub-group mask is not supported on host device"};
// Groups are not user-constructible, this call should not be reachable from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding llvm_unreachable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two concerns here:

  1. llvm_unreachable comes from LLVM headers that we don't ship (and probably don't want to)
  2. Any "early exits" in form of exception/unreachable may affect host compilation - I'm afraid of unintended side effects like we saw in [SYCL] Fix SYCL_EXTERNAL device code when linking with a static lib #14256

@AlexeySachkov AlexeySachkov merged commit 584e3d9 into intel:sycl Sep 12, 2025
28 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/simplify-sub-group-mask-header branch September 12, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants